-
Notifications
You must be signed in to change notification settings - Fork 190
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
digest: expose AssociatedAlgorithmIdentifier
through CoreWrapper
#1575
Conversation
@@ -20,6 +20,7 @@ block-buffer = { version = "=0.11.0-pre.5", optional = true } | |||
subtle = { version = "2.4", default-features = false, optional = true } | |||
blobby = { version = "0.3", optional = true } | |||
const-oid = { version = "=0.10.0-pre.2", optional = true } | |||
spki = { version = "=0.8.0-pre.0", optional = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, this has a circular dependency: spki
has an optional dependency on sha2
for computing SPKI fingerprints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, I missed this one.
Newtype then ? :D
And we get the AssociatedOid
and AssociatedAlgorithmIdentifier
attached to those ? :D
(I agree with the usability comment you made over #1069)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we had newtypes, it would at least get rid of any coupling between spki
and digest
, though the problem would remain for sha2
.
We could potentially split off an e.g. spki-fingerprint
crate so spki
doesn't depend on sha2
. It is something of a niche feature.
As a meta comment: I would once again suggest making newtypes, rather than This seems like a lot of unnecessary coupling through The type aliases are not semantically meaningful. It feels like internals leaking out. It's also very, very confusing from a diagnostics perspective. See #1069. |
It may be worth to experiment with newtype macros. I think we should still expose block-level types and provide ways to do conversion in both directions, but for most users plain newtypes should be indeed easier to understand. We also need to decide what should be done with variable size hashes. For example, I don't think we should introduce |
Would suggest closing this for now.
@newpavlov that seems fine to me. Curious with |
With |
@newpavlov you could potentially use the |
That would be |
Yes, I meant |
This allows implementation in consumer crates like:
RustCrypto/hashes#586